Rel 1.15 bp 3233 2#3241
Conversation
* Adds ImageSpec.with_dev_dependencies Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Add tests for noop builder Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Use runtime_packages Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Add docs abount how to use runtime packages Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Less diffs Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix formatting Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix docstring Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Dix docstring Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Let pip default to user by itself to be more compatible Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> --------- Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
* Image spec builder options Provide the ability to specify image `builder` specific options per Image Spec. Signed-off-by: Mike Hotan <mike@union.ai> * Add builder_options validation Signed-off-by: Mike Hotan <mike@union.ai> * updates Signed-off-by: Mike Hotan <mike@union.ai> --------- Signed-off-by: Mike Hotan <mike@union.ai>
wild-endeavor
requested review from
Future-Outlier,
cosmicBboy,
davidmirror-ops,
eapolinario,
kumare3,
pingsutw,
samhita-alla and
thomasjpfan
as code owners
Code Review Agent Run #5ddb1bActionable Suggestions - 2
Additional Suggestions - 10
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
| def _run_subprocess(cmd: List[str], env: Optional[dict] = None) -> int: | ||
| """Run cmd with proper SIGTERM handling.""" | ||
| p = subprocess.Popen(cmd, env=env) | ||
|
|
||
| def handle_sigterm(signum, frame): | ||
| logger.info(f"passing signum {signum} [frame={frame}] to subprocess") |
There was a problem hiding this comment.
Security issue on line 68: subprocess.Popen(cmd, env=env) may execute untrusted input. Consider validating the cmd parameter or using shlex.quote() if the command includes user input.
Code suggestion
Check the AI-generated fix before applying
| def _run_subprocess(cmd: List[str], env: Optional[dict] = None) -> int: | |
| """Run cmd with proper SIGTERM handling.""" | |
| p = subprocess.Popen(cmd, env=env) | |
| def handle_sigterm(signum, frame): | |
| logger.info(f"passing signum {signum} [frame={frame}] to subprocess") | |
| def _run_subprocess(cmd: List[str], env: Optional[dict] = None) -> int: | |
| """Run cmd with proper SIGTERM handling.""" | |
| # Ensure cmd is a list of strings to prevent shell injection | |
| if not all(isinstance(arg, str) for arg in cmd): | |
| raise ValueError("All command arguments must be strings") | |
| p = subprocess.Popen(cmd, env=env, shell=False) | |
| def handle_sigterm(signum, frame): | |
| logger.info(f"passing signum {signum} [frame={frame}] to subprocess") |
Code Review Run #5ddb1b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| @@ -95,6 +102,8 @@ class ImageSpec: | |||
| source_copy_mode: Optional[CopyFileDetection] = None | |||
| copy: Optional[List[str]] = None | |||
There was a problem hiding this comment.
The _update_attribute method now handles dictionaries but with_builder_options method doesn't properly initialize the builder_options attribute when it's None. This could cause a TypeError when trying to update a non-existent dictionary.
Code suggestion
Check the AI-generated fix before applying
| copy: Optional[List[str]] = None | |
| copy: Optional[List[str]] = None | |
| builder_options: Optional[Dict[str, Any]] = None |
Code Review Run #5ddb1b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3241 +/- ##
===========================================
+ Coverage 52.34% 94.75% +42.40%
===========================================
Files 213 55 -158
Lines 22312 2077 -20235
Branches 2916 0 -2916
===========================================
- Hits 11680 1968 -9712
+ Misses 9946 109 -9837
+ Partials 686 0 -686 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
Summary by Bito
The pull request enhances Flytekit with improved subprocess signal handling, image specification capabilities, and runtime package management features. It introduces NoOpBuilder for image building and adds the RUNTIME_PACKAGES_ENV_NAME constant. The changes improve test coverage and update documentation baselines to reflect current code structure.Unit tests added: False
Estimated effort to review (1-5, lower is better): 3